refactor: extract shared UI components (Phase 08)#822
refactor: extract shared UI components (Phase 08)#822reachrazamair merged 4 commits intoRunMaestro:rcfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds three UI primitives— Changes
Sequence Diagram(s)(Skipped — UI primitive additions and widespread replacements do not introduce a new multi-component sequential control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3d4c050 to
eb0eb0b
Compare
Greptile SummaryThis PR extracts three shared UI primitives — Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph "New Shared Primitives (ui/)"
GIB["GhostIconButton\n(forwardRef, stopPropagation,\ncolor, padding, testId)"]
SP["Spinner\n(Loader2 + animate-spin,\nsize, color, ariaLabel)"]
ESP["EmptyStatePlaceholder\n(icon, title, description,\naction, padding props)"]
IDX["ui/index.ts\n(barrel export)"]
end
subgraph "54 migrated call sites"
MOD["Modal.tsx (header X btn)"]
ASB["AgentSessionsBrowser.tsx\n(partial: 4 GhostIconButtons +\n2 residual Loader2)"]
ASM["AgentSessionsModal.tsx\n(partial: residual Loader2)"]
OTH["41 other files"]
end
subgraph "84 migrated Spinner sites"
UCM["UpdateCheckModal.tsx (full)"]
SYM["SymphonyModal.tsx\n(partial: residual Loader2)"]
NP["NotificationsPanel.tsx\n(partial: residual Loader2)"]
OTH2["51 other files"]
end
subgraph "2 EmptyStatePlaceholder sites"
ASB2["AgentSessionsBrowser.tsx"]
ASM2["AgentSessionsModal.tsx"]
end
subgraph "Renamed (collision resolution)"
MER["MergeProgressModal\nSpinner→CircularSpinner"]
SUM["SummarizeProgressModal\nSpinner→CircularSpinner"]
TRA["TransferProgressModal\nSpinner→CircularSpinner"]
end
IDX --> GIB
IDX --> SP
IDX --> ESP
GIB --> MOD
GIB --> ASB
GIB --> ASM
GIB --> OTH
SP --> UCM
SP --> SYM
SP --> NP
SP --> OTH2
ESP --> ASB2
ESP --> ASM2
SP -.->|"could replace"| MER
SP -.->|"could replace"| SUM
SP -.->|"could replace"| TRA
|
| export function Spinner({ | ||
| size = 16, | ||
| color, | ||
| className = '', | ||
| style, | ||
| ariaLabel = 'Loading', | ||
| }: SpinnerProps) { | ||
| return ( | ||
| <Loader2 | ||
| className={`animate-spin ${className}`.trim()} | ||
| style={{ width: size, height: size, color, ...style }} | ||
| aria-label={ariaLabel} | ||
| role="status" | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
No
testId prop unlike GhostIconButton
GhostIconButton exposes a testId prop (data-testid={testId}), but Spinner doesn't. Any test that needs to locate a Spinner must reach through the implementation detail (data-testid="loader2-icon" from the lucide mock), which will silently break if the underlying icon ever changes. Adding a testId prop (or a stable role="status" query approach) would keep test selectors decoupled from internal icon choices.
| export function Spinner({ | |
| size = 16, | |
| color, | |
| className = '', | |
| style, | |
| ariaLabel = 'Loading', | |
| }: SpinnerProps) { | |
| return ( | |
| <Loader2 | |
| className={`animate-spin ${className}`.trim()} | |
| style={{ width: size, height: size, color, ...style }} | |
| aria-label={ariaLabel} | |
| role="status" | |
| /> | |
| ); | |
| } | |
| export interface SpinnerProps { | |
| /** Icon size in pixels. Defaults to 16 */ | |
| size?: number; | |
| /** Optional color (applied via inline style). */ | |
| color?: string; | |
| /** Additional class names */ | |
| className?: string; | |
| /** Inline style overrides */ | |
| style?: CSSProperties; | |
| /** Accessible label. Defaults to 'Loading' */ | |
| ariaLabel?: string; | |
| /** Test id for automated tests */ | |
| testId?: string; | |
| } | |
| export function Spinner({ | |
| size = 16, | |
| color, | |
| className = '', | |
| style, | |
| ariaLabel = 'Loading', | |
| testId, | |
| }: SpinnerProps) { | |
| return ( | |
| <Loader2 | |
| className={`animate-spin ${className}`.trim()} | |
| style={{ width: size, height: size, color, ...style }} | |
| aria-label={ariaLabel} | |
| role="status" | |
| data-testid={testId} | |
| /> | |
| ); | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/SymphonyModal.tsx (1)
712-719:⚠️ Potential issue | 🟡 MinorRemove unreachable loading indicator in the Available Issues header.
Line 718 is unreachable in this branch: Line 671 already routes
isLoadingIssues === trueto the skeleton path, so this conditional spinner never renders.Suggested cleanup
- {isLoadingIssues && <Spinner size={12} color={theme.colors.accent} />}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SymphonyModal.tsx` around lines 712 - 719, The header for "Available Issues" contains an unreachable loading indicator: remove the conditional JSX `{isLoadingIssues && <Spinner size={12} color={theme.colors.accent} />}` in the h4 (the Available Issues header) because `isLoadingIssues` is already routed to the skeleton path earlier; update the component (SymphonyModal.tsx) to simply render the header text and count (availableIssues.length) without that spinner conditional.
🧹 Nitpick comments (6)
src/renderer/components/ui/GhostIconButton.tsx (1)
31-32: Add an accessibility fallback for icon-only buttons.If
ariaLabelis omitted, this can render an unlabeled icon button. Fallback totitlehelps keep controls accessible with existing call sites.Proposed fix
- aria-label={ariaLabel} + aria-label={ariaLabel ?? title}Also applies to: 92-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/ui/GhostIconButton.tsx` around lines 31 - 32, The GhostIconButton component currently allows an icon-only button to be rendered without an accessible name; update the rendered element to set aria-label to a fallback value by using the props ariaLabel || title (i.e., when ariaLabel is absent use title) so the button always has an accessible name, and ensure the title prop is preserved on the element; locate GhostIconButton and change where aria-label is assigned to use ariaLabel ?? title (or ariaLabel || title) for both occurrences referenced (around the ariaLabel prop and the other usage).src/renderer/components/SendToAgentModal.tsx (1)
18-19: Consider completing spinner standardization in this modal.Line 19 introduces the shared Spinner, but the send CTA still uses
Loader2(Line 721). Converging on one loader component here will keep UI behavior/styles consistent and simplify imports.♻️ Suggested patch
-import { Search, ArrowRight, X, Loader2, Circle } from 'lucide-react'; +import { Search, ArrowRight, X, Circle } from 'lucide-react'; @@ {isSending ? ( <> - <Loader2 className="w-4 h-4 animate-spin" aria-hidden="true" /> + <Spinner size={16} /> Sending... </> ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SendToAgentModal.tsx` around lines 18 - 19, The send CTA in SendToAgentModal mixes two loader components—use the shared Spinner instead of Loader2 to standardize UI; update the send button render path in the SendToAgentModal component to import and render Spinner (already imported at top) wherever Loader2 is used (e.g., the send CTA/submit handler render branch), remove the Loader2 usage and any related imports, and ensure Spinner receives the same props (size/weight/className) used by Loader2 so styling and behavior remain consistent.src/renderer/components/Settings/EnvVarsEditor.tsx (1)
194-201: PreferariaLabelon the icon-only remove control.Line 194 is functionally correct; adding
ariaLabelmakes the remove action more reliably announced by screen readers.♿ Suggested patch
<GhostIconButton onClick={() => removeEntry(entry.id)} padding="p-2" title="Remove variable" + ariaLabel={`Remove variable ${entry.key || ''}`.trim()} color={theme.colors.textDim} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Settings/EnvVarsEditor.tsx` around lines 194 - 201, The GhostIconButton used for removing env var entries (the element rendering Trash2 with onClick={() => removeEntry(entry.id)}) is icon-only and should include an accessible label; add an ariaLabel prop (e.g., ariaLabel="Remove variable") to the GhostIconButton so screen readers reliably announce the action, keeping the existing title and styling intact and ensuring the removeEntry(entry.id) handler remains unchanged.src/renderer/components/DocumentsPanel.tsx (1)
488-490: Add an explicit accessible label to the icon-only close button.Line 488 currently renders only an icon; adding
ariaLabel(and optionallytitle) makes the control clearer for assistive tech.♿ Suggested patch
- <GhostIconButton onClick={onClose} color={theme.colors.textDim}> + <GhostIconButton + onClick={onClose} + color={theme.colors.textDim} + ariaLabel="Close document selector" + title="Close" + > <X className="w-4 h-4" /> </GhostIconButton>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/DocumentsPanel.tsx` around lines 488 - 490, The close button is icon-only and needs an accessible name: update the GhostIconButton usage (the one with onClose and the X icon) to pass an explicit accessible label like aria-label="Close" (and optional title="Close") so screen readers can announce the control; ensure the props are added to the GhostIconButton component invocation that currently renders <GhostIconButton onClick={onClose} color={theme.colors.textDim}> with the X icon inside.src/renderer/components/PlaygroundPanel.tsx (1)
599-601: Add an accessible name to the playground close icon button.Line 599 is icon-only; include
ariaLabelso assistive tech can announce the action reliably.♿ Suggested patch
- <GhostIconButton onClick={onClose} color={theme.colors.textDim}> + <GhostIconButton + onClick={onClose} + color={theme.colors.textDim} + ariaLabel="Close developer playground" + title="Close" + > <X className="w-5 h-5" /> </GhostIconButton>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/PlaygroundPanel.tsx` around lines 599 - 601, The close button in PlaygroundPanel is icon-only (GhostIconButton with X) and needs an accessible name; update the GhostIconButton instantiation in PlaygroundPanel.tsx to pass an accessible-name prop (e.g., ariaLabel or aria-label depending on the GhostIconButton prop API) such as "Close playground" and keep the existing onClose and color props so assistive tech can announce the action reliably.src/renderer/components/AutoRun/AutoRunExpandedModal.tsx (1)
422-424: Consider addingariaLabelexplicitly on the close icon button.At Line 422, this currently relies on
titlefor naming. AddingariaLabelimproves consistency and avoids depending on tooltip semantics for accessible naming.Suggested refinement
- <GhostIconButton onClick={handleClose} title="Close (Esc)"> + <GhostIconButton + onClick={handleClose} + title="Close (Esc)" + ariaLabel="Close Auto Run expanded dialog" + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AutoRun/AutoRunExpandedModal.tsx` around lines 422 - 424, The close button relies on a title for accessible naming; update the GhostIconButton instance used with the onClick handler handleClose and icon X to include an explicit ariaLabel prop (e.g., ariaLabel="Close") so screen readers get a consistent name rather than depending on the tooltip/title; ensure the ariaLabel string matches other close buttons in the component for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/renderer/components/ui/Spinner.test.tsx`:
- Around line 12-32: Tests are failing because the Spinner component does not
expose data-testid="loader2-icon" while the tests (Spinner.test.tsx) query that
id; update the Spinner component (Spinner in
src/renderer/components/ui/Spinner.tsx) to add data-testid="loader2-icon" to the
rendered icon element (the same element that receives className, style/size and
color props) so the test queries (screen.getByTestId('loader2-icon')) find the
element, ensuring the component still applies className, size, and color props
to that element.
In `@src/renderer/components/AgentSessionsBrowser.tsx`:
- Around line 732-739: The icon-only GhostIconButton used to trigger
clearViewingSession lacks accessible naming; update the GhostIconButton instance
(the one rendering ChevronLeft and calling clearViewingSession) to include an
accessible label by adding an ariaLabel (e.g., "Back" or "Close session") and a
title prop so screen readers and tooltips can announce the button; ensure the
label accurately describes the action (e.g., ariaLabel="Go back to session
list") and keep the existing onClick and styling unchanged.
In `@src/renderer/components/AgentSessionsModal.tsx`:
- Around line 449-457: The GhostIconButton used as the icon-only back control in
AgentSessionsModal lacks an accessible name; update the GhostIconButton
invocation (the instance rendering ChevronLeft inside AgentSessionsModal) to
include an accessible label (e.g., aria-label="Back" or aria-label="Go back to
sessions" and optionally title="Back") so assistive technology can announce the
control; keep the existing onClick behavior that calls setViewingSession(null)
and setMessages([]).
- Around line 601-604: The empty-state title in AgentSessionsModal currently
hardcodes “Claude” (the ternary setting for title when sessions.length === 0);
change it to a provider-agnostic message or use the modal's provider variable
(e.g., providerName or session.provider) so the text reads generically like "No
sessions found for this project" or interpolates the current provider
dynamically; update the title prop in the AgentSessionsModal render branch that
references sessions to use the generic string or the provider variable instead
of the literal "Claude".
In `@src/renderer/components/CreatePRModal.tsx`:
- Around line 233-235: The close button is an icon-only control
(GhostIconButton) with no accessible name; update the GhostIconButton instance
used for closing the CreatePRModal to include an accessible label (e.g.,
ariaLabel="Close create pull request modal" or aria-label="Close") so assistive
tech can identify it; add the prop to the GhostIconButton wrapping the X icon
(the onClose handler stays unchanged) and use a clear, concise label like
"Close" or "Close create PR modal".
In `@src/renderer/components/CreateWorktreeModal.tsx`:
- Around line 143-145: The close button is icon-only and lacks an accessible
name; update the GhostIconButton element (the instance with onClose and child X)
to provide an accessible label (e.g., add aria-label="Close" and a matching
title="Close" or include visually hidden text) so screen readers can announce
it; ensure the label text is concise and localized if needed and keep the
onClick handler (onClose) unchanged.
In `@src/renderer/components/DirectorNotes/DirectorNotesModal.tsx`:
- Around line 215-217: The close button rendered with GhostIconButton (wrapping
the X icon and invoking onClose) lacks an accessible name; update the
GhostIconButton usage to include an aria-label (e.g., aria-label="Close" or a
localized string like aria-label={t('close')}) and also add a title attribute if
a tooltip is desired so screen readers and sighted users get a clear label;
ensure you set the attribute on the GhostIconButton element (not on the X icon)
to preserve existing behavior.
In `@src/renderer/components/LeaderboardRegistrationModal.tsx`:
- Around line 801-803: The GhostIconButton used for closing (the element with
onClick={onClose} containing the <X /> icon) is icon-only and lacks an
accessible name; update the GhostIconButton invocation to provide an explicit
accessible label by adding an ariaLabel (or aria-label) prop such as "Close"
(and optionally a title prop) so screen readers can announce its purpose,
ensuring the button still uses theme.colors.textDim for styling.
In `@src/renderer/components/MainPanel/MainPanelHeader.tsx`:
- Around line 225-236: The GhostIconButton used to copy the branch name (the
component wrapping the Copy icon and calling copyToClipboard with
gitInfo.branch) relies only on the title prop for accessibility; add an explicit
ariaLabel prop (e.g., "Copy branch name") to GhostIconButton so the icon-only
control has a proper accessible name for screen readers, keeping the existing
title and click behavior intact.
In `@src/renderer/components/Settings/SshRemoteModal.tsx`:
- Around line 764-771: The icon-only GhostIconButton used to remove env vars
(the one wrapping <Trash2 /> and calling removeEnvVar(entry.id)) needs an
explicit accessible name: add an ariaLabel prop (or aria-label if that is the
component API) with a descriptive string such as "Remove variable" plus the
variable identifier (e.g., entry.key or entry.id) so screen readers can announce
which variable will be removed when the button is activated.
In `@src/renderer/components/ShortcutsHelpModal.tsx`:
- Around line 75-77: The GhostIconButton used for closing the ShortcutsHelpModal
is icon-only and lacks an accessible name; update the JSX for GhostIconButton
(the instance with props onClose and child <X className="w-4 h-4" />) to include
an ariaLabel (e.g., ariaLabel="Close shortcuts help" or similar) and optionally
a title prop with the same text so screen readers and tooltips have a clear
label; ensure the ariaLabel string is concise and descriptive of the control's
action.
In `@src/renderer/components/TransferProgressModal.tsx`:
- Around line 386-393: The close-handler currently uses onComplete?.() ||
onCancel(), which calls onCancel after onComplete because onComplete returns
void (falsy); change the handler in TransferProgressModal's GhostIconButton so
it conditionally calls onComplete if defined else calls onCancel (e.g., if
(onComplete) { onComplete(); } else { onCancel(); }) ensuring only one callback
runs when the close button is clicked; update references to onComplete and
onCancel in that GhostIconButton onClick prop accordingly.
In `@src/renderer/components/UpdateCheckModal.tsx`:
- Around line 195-197: The GhostIconButton rendering the close icon
(GhostIconButton with onClose and inner <X />) is missing an accessible label;
update the GhostIconButton usage in UpdateCheckModal to include an explicit
aria-label (e.g., "Close update modal") or ariaLabel prop (or title) so screen
readers can identify the control, ensuring the close action remains wired to
onClose and the icon (<X />) is unchanged.
In `@src/renderer/components/WorktreeConfigModal.tsx`:
- Around line 209-211: The close icon button in WorktreeConfigModal uses
GhostIconButton with only an <X /> icon and no accessible name; update the
GhostIconButton where it is rendered (the element with props onClick={onClose}
and child <X />) to include an explicit accessible name by adding an aria-label
(e.g., aria-label="Close") or a title prop so screen readers can announce it;
ensure the prop key matches GhostIconButton's API (aria-label or title) so
accessibility tools receive the label.
---
Outside diff comments:
In `@src/renderer/components/SymphonyModal.tsx`:
- Around line 712-719: The header for "Available Issues" contains an unreachable
loading indicator: remove the conditional JSX `{isLoadingIssues && <Spinner
size={12} color={theme.colors.accent} />}` in the h4 (the Available Issues
header) because `isLoadingIssues` is already routed to the skeleton path
earlier; update the component (SymphonyModal.tsx) to simply render the header
text and count (availableIssues.length) without that spinner conditional.
---
Nitpick comments:
In `@src/renderer/components/AutoRun/AutoRunExpandedModal.tsx`:
- Around line 422-424: The close button relies on a title for accessible naming;
update the GhostIconButton instance used with the onClick handler handleClose
and icon X to include an explicit ariaLabel prop (e.g., ariaLabel="Close") so
screen readers get a consistent name rather than depending on the tooltip/title;
ensure the ariaLabel string matches other close buttons in the component for
consistency.
In `@src/renderer/components/DocumentsPanel.tsx`:
- Around line 488-490: The close button is icon-only and needs an accessible
name: update the GhostIconButton usage (the one with onClose and the X icon) to
pass an explicit accessible label like aria-label="Close" (and optional
title="Close") so screen readers can announce the control; ensure the props are
added to the GhostIconButton component invocation that currently renders
<GhostIconButton onClick={onClose} color={theme.colors.textDim}> with the X icon
inside.
In `@src/renderer/components/PlaygroundPanel.tsx`:
- Around line 599-601: The close button in PlaygroundPanel is icon-only
(GhostIconButton with X) and needs an accessible name; update the
GhostIconButton instantiation in PlaygroundPanel.tsx to pass an accessible-name
prop (e.g., ariaLabel or aria-label depending on the GhostIconButton prop API)
such as "Close playground" and keep the existing onClose and color props so
assistive tech can announce the action reliably.
In `@src/renderer/components/SendToAgentModal.tsx`:
- Around line 18-19: The send CTA in SendToAgentModal mixes two loader
components—use the shared Spinner instead of Loader2 to standardize UI; update
the send button render path in the SendToAgentModal component to import and
render Spinner (already imported at top) wherever Loader2 is used (e.g., the
send CTA/submit handler render branch), remove the Loader2 usage and any related
imports, and ensure Spinner receives the same props (size/weight/className) used
by Loader2 so styling and behavior remain consistent.
In `@src/renderer/components/Settings/EnvVarsEditor.tsx`:
- Around line 194-201: The GhostIconButton used for removing env var entries
(the element rendering Trash2 with onClick={() => removeEntry(entry.id)}) is
icon-only and should include an accessible label; add an ariaLabel prop (e.g.,
ariaLabel="Remove variable") to the GhostIconButton so screen readers reliably
announce the action, keeping the existing title and styling intact and ensuring
the removeEntry(entry.id) handler remains unchanged.
In `@src/renderer/components/ui/GhostIconButton.tsx`:
- Around line 31-32: The GhostIconButton component currently allows an icon-only
button to be rendered without an accessible name; update the rendered element to
set aria-label to a fallback value by using the props ariaLabel || title (i.e.,
when ariaLabel is absent use title) so the button always has an accessible name,
and ensure the title prop is preserved on the element; locate GhostIconButton
and change where aria-label is assigned to use ariaLabel ?? title (or ariaLabel
|| title) for both occurrences referenced (around the ariaLabel prop and the
other usage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c87ed85-2ca9-404a-933f-773a4adb2b8e
📒 Files selected for processing (76)
src/__tests__/renderer/components/ui/EmptyStatePlaceholder.test.tsxsrc/__tests__/renderer/components/ui/GhostIconButton.test.tsxsrc/__tests__/renderer/components/ui/Spinner.test.tsxsrc/renderer/components/AboutModal.tsxsrc/renderer/components/AgentCreationDialog.tsxsrc/renderer/components/AgentPromptComposerModal.tsxsrc/renderer/components/AgentSessionsBrowser.tsxsrc/renderer/components/AgentSessionsModal.tsxsrc/renderer/components/AutoRun/AttachmentImage.tsxsrc/renderer/components/AutoRun/AutoRunExpandedModal.tsxsrc/renderer/components/AutoRun/AutoRunSearchBar.tsxsrc/renderer/components/AutoRun/AutoRunToolbar.tsxsrc/renderer/components/BatchRunnerModal.tsxsrc/renderer/components/CreatePRModal.tsxsrc/renderer/components/CreateWorktreeModal.tsxsrc/renderer/components/CueYamlEditor/CueAiChat.tsxsrc/renderer/components/CueYamlEditor/CueYamlEditor.tsxsrc/renderer/components/DebugPackageModal.tsxsrc/renderer/components/DeleteWorktreeModal.tsxsrc/renderer/components/DirectorNotes/AIOverviewTab.tsxsrc/renderer/components/DirectorNotes/DirectorNotesModal.tsxsrc/renderer/components/DirectorNotes/UnifiedHistoryTab.tsxsrc/renderer/components/DocumentGraph/DocumentGraphView.tsxsrc/renderer/components/DocumentsPanel.tsxsrc/renderer/components/EmptyStateView.tsxsrc/renderer/components/FeedbackChatView.tsxsrc/renderer/components/FileExplorerPanel.tsxsrc/renderer/components/FilePreview/FilePreview.tsxsrc/renderer/components/FilePreview/FilePreviewHeader.tsxsrc/renderer/components/FilePreview/ImageViewer.tsxsrc/renderer/components/FilePreview/MarkdownImage.tsxsrc/renderer/components/GistPublishModal.tsxsrc/renderer/components/GitWorktreeSection.tsxsrc/renderer/components/GroupChatModal.tsxsrc/renderer/components/History/HistoryStatsBar.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView.tsxsrc/renderer/components/InlineWizard/WizardPill.tsxsrc/renderer/components/LeaderboardRegistrationModal.tsxsrc/renderer/components/MainPanel/AgentErrorBanner.tsxsrc/renderer/components/MainPanel/BrowserTabView.tsxsrc/renderer/components/MainPanel/MainPanelContent.tsxsrc/renderer/components/MainPanel/MainPanelHeader.tsxsrc/renderer/components/MarkdownRenderer.tsxsrc/renderer/components/MarketplaceModal.tsxsrc/renderer/components/MergeProgressModal.tsxsrc/renderer/components/MergeProgressOverlay.tsxsrc/renderer/components/MergeSessionModal.tsxsrc/renderer/components/NewInstanceModal/AgentPickerGrid.tsxsrc/renderer/components/NewInstanceModal/EditAgentModal.tsxsrc/renderer/components/NotificationsPanel.tsxsrc/renderer/components/PlaygroundPanel.tsxsrc/renderer/components/PromptComposerModal.tsxsrc/renderer/components/RightPanel.tsxsrc/renderer/components/SendToAgentModal.tsxsrc/renderer/components/SessionItem.tsxsrc/renderer/components/SessionList/SessionList.tsxsrc/renderer/components/Settings/EnvVarsEditor.tsxsrc/renderer/components/Settings/SettingsSearch.tsxsrc/renderer/components/Settings/SshRemoteModal.tsxsrc/renderer/components/Settings/SshRemotesSection.tsxsrc/renderer/components/ShortcutsHelpModal.tsxsrc/renderer/components/SummarizeProgressModal.tsxsrc/renderer/components/SummarizeProgressOverlay.tsxsrc/renderer/components/SymphonyModal.tsxsrc/renderer/components/ToolCallCard.tsxsrc/renderer/components/TransferProgressModal.tsxsrc/renderer/components/UpdateCheckModal.tsxsrc/renderer/components/Wizard/screens/PhaseReviewScreen.tsxsrc/renderer/components/Wizard/shared/DocumentEditor.tsxsrc/renderer/components/WorktreeConfigModal.tsxsrc/renderer/components/shared/AgentConfigPanel.tsxsrc/renderer/components/ui/EmptyStatePlaceholder.tsxsrc/renderer/components/ui/GhostIconButton.tsxsrc/renderer/components/ui/Modal.tsxsrc/renderer/components/ui/Spinner.tsxsrc/renderer/components/ui/index.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/renderer/components/AgentSessionsBrowser.tsx (1)
741-761: AddariaLabelto remaining icon-onlyGhostIconButtoncontrols.These buttons currently rely on
titleonly. AddingariaLabelkeeps accessible naming explicit and consistent across icon-only actions.♿ Suggested tweak
<GhostIconButton onClick={(e) => toggleStar(viewingSession.sessionId, e)} padding="p-1.5" + ariaLabel={ + starredSessions.has(viewingSession.sessionId) + ? 'Remove from favorites' + : 'Add to favorites' + } title={ starredSessions.has(viewingSession.sessionId) ? 'Remove from favorites' : 'Add to favorites' } > @@ <GhostIconButton onClick={(e) => { e.stopPropagation(); setRenamingSessionId(viewingSession.sessionId); setRenameValue(viewingSession.sessionName || ''); setTimeout(() => renameInputRef.current?.focus(), 50); }} padding="p-0.5" + ariaLabel="Rename session" title="Rename session" > @@ <GhostIconButton onClick={(e) => { e.stopPropagation(); setRenamingSessionId(viewingSession.sessionId); setRenameValue(''); setTimeout(() => renameInputRef.current?.focus(), 50); }} padding="p-0.5" + ariaLabel="Add session name" title="Add session name" >Also applies to: 799-810, 821-832
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AgentSessionsBrowser.tsx` around lines 741 - 761, The GhostIconButton instances used for starring sessions (the component rendering Star and calling toggleStar with viewingSession.sessionId and using title prop) lack explicit accessible names; add an ariaLabel prop to each icon-only GhostIconButton (set it to the same string as the title, i.e., "Add to favorites" or "Remove from favorites" based on starredSessions.has(viewingSession.sessionId)) so screen readers receive an explicit label; apply the same change to the other GhostIconButton occurrences handling session actions in the same file that use icon-only buttons.src/renderer/components/TransferProgressModal.tsx (1)
387-393: Optional: reuse existing handler to avoid callback-branch duplication.This branch is repeated in multiple places; wiring this button to
handlePrimaryClick(or a shared helper) would reduce drift risk.♻️ Minimal dedup option
{isComplete && ( <GhostIconButton - onClick={() => { - if (onComplete) { - onComplete(); - } else { - onCancel(); - } - }} + onClick={handlePrimaryClick} ariaLabel="Close modal" color={theme.colors.textDim} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/TransferProgressModal.tsx` around lines 387 - 393, The click handler duplicates logic present in handlePrimaryClick; replace the inline onClick block in TransferProgressModal (currently calling onComplete() or onCancel()) with a call to handlePrimaryClick to reuse the shared behavior and avoid branching duplication—ensure handlePrimaryClick is accessible in the component scope and that it performs the same onComplete/onCancel decision so behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/components/AgentSessionsBrowser.tsx`:
- Around line 741-761: The GhostIconButton instances used for starring sessions
(the component rendering Star and calling toggleStar with
viewingSession.sessionId and using title prop) lack explicit accessible names;
add an ariaLabel prop to each icon-only GhostIconButton (set it to the same
string as the title, i.e., "Add to favorites" or "Remove from favorites" based
on starredSessions.has(viewingSession.sessionId)) so screen readers receive an
explicit label; apply the same change to the other GhostIconButton occurrences
handling session actions in the same file that use icon-only buttons.
In `@src/renderer/components/TransferProgressModal.tsx`:
- Around line 387-393: The click handler duplicates logic present in
handlePrimaryClick; replace the inline onClick block in TransferProgressModal
(currently calling onComplete() or onCancel()) with a call to handlePrimaryClick
to reuse the shared behavior and avoid branching duplication—ensure
handlePrimaryClick is accessible in the component scope and that it performs the
same onComplete/onCancel decision so behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c6a6d9f-1b25-4267-8b86-d11fe6a2095b
📒 Files selected for processing (12)
src/renderer/components/AgentSessionsBrowser.tsxsrc/renderer/components/AgentSessionsModal.tsxsrc/renderer/components/CreatePRModal.tsxsrc/renderer/components/CreateWorktreeModal.tsxsrc/renderer/components/DirectorNotes/DirectorNotesModal.tsxsrc/renderer/components/LeaderboardRegistrationModal.tsxsrc/renderer/components/MainPanel/MainPanelHeader.tsxsrc/renderer/components/Settings/SshRemoteModal.tsxsrc/renderer/components/ShortcutsHelpModal.tsxsrc/renderer/components/TransferProgressModal.tsxsrc/renderer/components/UpdateCheckModal.tsxsrc/renderer/components/WorktreeConfigModal.tsx
✅ Files skipped from review due to trivial changes (1)
- src/renderer/components/CreateWorktreeModal.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/renderer/components/ShortcutsHelpModal.tsx
- src/renderer/components/DirectorNotes/DirectorNotesModal.tsx
- src/renderer/components/CreatePRModal.tsx
- src/renderer/components/MainPanel/MainPanelHeader.tsx
- src/renderer/components/LeaderboardRegistrationModal.tsx
- src/renderer/components/AgentSessionsModal.tsx
b6e6d60 to
24b840c
Compare
- 08A: Extract GhostIconButton (src/renderer/components/ui/GhostIconButton.tsx) encapsulating the "p-N rounded hover:bg-white/10 transition-colors" icon-button pattern. Migrated 54 call sites across 43 files via AST-aware codemod. - 08B: Extract Spinner (src/renderer/components/ui/Spinner.tsx) for the Loader2 + animate-spin loading indicator pattern. Migrated 84 call sites across 55 files and removed 42 now-unused Loader2 imports. - 08C: Add EmptyStatePlaceholder (src/renderer/components/ui/EmptyStatePlaceholder.tsx) for the generic "No X" icon + title + description pattern. Adopted at 2 call sites (AgentSessionsBrowser, AgentSessionsModal). Distinct from the top-level EmptyStateView welcome screen. Added minimal vitest coverage for each new primitive (13 tests total). All three components exported from src/renderer/components/ui/index.ts. Net impact: 70 files changed, 404 insertions(+), 582 deletions(-).
- Fix onComplete?.() || onCancel() always calling both (void is falsy) - Add ariaLabel to icon-only GhostIconButton instances across 11 modals - Remove hardcoded "Claude" from AgentSessionsModal empty state
8d292a0 to
d96adf6
Compare
Summary
Extracts three shared UI primitives and migrates existing call sites. Net effect: one canonical pattern per primitive instead of 140+ hand-rolled variants.
Net: +223 lines overall (new components + tests), but -178 lines in migrated call sites (3 new component files + tests account for the delta)
08A - GhostIconButton
New:
src/renderer/components/ui/GhostIconButton.tsx+ test.The standard "icon-only button with hover bg and optional tooltip" pattern used 100+ times across the renderer.
classNamedrop,aria-label->ariaLabel,style={{color:X}}->color={X})08B - Spinner
New:
src/renderer/components/ui/Spinner.tsx+ test.The standard animated loading indicator wrapping
Loader2from lucide-react.Spinnerthat did something different (CSS-driven circular progress) - renamed those toCircularSpinnerto resolve the name collision08C - EmptyStatePlaceholder
New:
src/renderer/components/ui/EmptyStatePlaceholder.tsx+ test.A generic placeholder for "No X" / "No results" / "Select something" states. Intentionally distinct from the existing
EmptyStateView.tsx, which is the specialized welcome-screen component - different purpose, would have needed a confusing mode prop.AgentSessionsBrowser,AgentSessionsModal)Test plan
npm run lintpasses (all 3 tsconfigs)npx prettier --check .passesRisk
Low. Behavior-preserving migration: each call site produces identical DOM/styling. Three new components with dedicated tests. One intentional name-rename (
Spinner->CircularSpinnerin 3 files) for the collision case.Cross-PR interaction
This PR modifies
AgentSessionsModal.tsx, which is deleted by #791 (Phase 01A). Whichever merges second will need a trivial conflict resolution (this PR's edits to that file will be dropped if 01A is already in).Summary by CodeRabbit
New Features
Refactor
Tests